Skip to content

feat: Implement Model Rewrite and Traffic Splitting Logic #1820

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

feat: Implement Model Rewrite and Traffic Splitting Logic #1820

wants to merge 3 commits into from
+1,167 −79

Conversation

@zetxqx
Copy link
Contributor

@zetxqx zetxqx commented Nov 5, 2025 •
edited
Loading

What type of PR is this?

/kind feature

What this PR does / why we need it:

This pull request introduces the core logic for model rewriting and weighted traffic
splitting within the request control director. It also includes the reconciler logic for InferenceModelRewrite resources.

Key changes:

  • pkg/epp/requestcontrol:
    • Adds the applyWeightedModelRewrite function to handle model rewriting based on
      InferenceModelRewrite rules.
    • Implements weighted selection of target models for traffic splitting.
    • Ensures that the oldest InferenceModelRewrite resource is respected in case of
      duplicate rules
  • pkg/epp/controller:
    • Implements the read-only reconciler logic for InferenceModelRewrite resources

Which issue(s) this PR fixes:

Fixes partially #1811

Does this PR introduce a user-facing change?:

Users can now configure `InferenceModelRewrite` resources to automatically redirect 
incoming model requests to different target models. This feature also supports weighted 
traffic splitting, allowing you to distribute requests across multiple target models based
 on defined percentages.

@k8s-ci-robot k8s-ci-robot added the kind/feature Categorizes issue or PR as related to a new feature. label Nov 5, 2025
@netlify
Copy link

netlify bot commented Nov 5, 2025 •
edited
Loading

✅ Deploy Preview for gateway-api-inference-extension ready!

Name Link
🔨 Latest commit dc0cca4
🔍 Latest deploy log https://app.netlify.com/projects/gateway-api-inference-extension/deploys/691d1ab4c081f80008f90267
😎 Deploy Preview https://deploy-preview-1820--gateway-api-inference-extension.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Nov 5, 2025

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: zetxqx
Once this PR has been reviewed and has the lgtm label, please assign nirrozenbaum for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Nov 5, 2025
@zetxqx zetxqx mentioned this pull request Nov 5, 2025
Merged
@zetxqx
Copy link
Contributor Author

zetxqx commented Nov 5, 2025

/retest

@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Nov 18, 2025
@zetxqx
Copy link
Contributor Author

zetxqx commented Nov 18, 2025

@ahg-g @kfswain rebased and should be ready for review now! thanks in advance!

nirrozenbaum
nirrozenbaum reviewed Nov 18, 2025
View reviewed changes
pkg/epp/datastore/datastore.go Outdated
// key: InferenceObjective name, value: *InferenceObjective
objectives map[string]*v1alpha2.InferenceObjective
// key: types.NamespacedName, value: *v1alpha2.InferenceModelRewrite
rewrites map[types.NamespacedName]*v1alpha2.InferenceModelRewrite
Copy link
Contributor

@nirrozenbaum nirrozenbaum Nov 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally I don't want to fetch ALL Rewrite objects on every request (assuming there will be objects that are model specific).
I think this should be reflected here in the way we store the objects.
keeping NamespacedName as the key in the name doesn't give us any "smart" mapping.
I think we should map from model_name -> rewrite object, while keeping empty model name as match for all requests. so on every request we call datastore to get the relevant rewrite objects and get back the:

  • specific rewrite objects for the requested model; and
  • the general rewrite objects that match all requests.

Copy link
Contributor Author

@zetxqx zetxqx Nov 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call, working on some patches to improve it will let you know when it's ready to review.

Copy link
Contributor Author

@zetxqx zetxqx Nov 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a new ModelRewriteStore for the modelRewrite, and making the rewrite request fetching efficiently. O(1). Adding ModelRewrite may not be very efficient but that operation should not happen very often.

Notably I changed the API rule conflict precedence rule , now:

  1. we'll always consider Exact match.
  2. if exact match not matching anything or multiple rules have exact match, we then compare the createTimestamp, and older rule wins.

@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Nov 19, 2025
zetxqx
zetxqx commented Nov 19, 2025
View reviewed changes
apix/v1alpha2/inferencemodelrewrite_types.go
Comment on lines +60 to 71
// Across all rules specified on applicable rewrites, precedence MUST be
// given to the match having an "Exact" model match over a generic match
// (a rule with an empty `matches` array).
//
// If ties still exist across multiple InferenceModelRewrite resources (e.g.
// two rewrites both have an exact match for the same model), matching
// precedence MUST be determined by the oldest resource based on
// creation timestamp.
//
// If ties still exist within a single InferenceModelRewrite resource, the
// FIRST matching rule (in list order) is used.
// +required
Copy link
Contributor Author

@zetxqx zetxqx Nov 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nirrozenbaum @ahg-g @kfswain

I've updated the precedence rules for conflicting matches to better align with the HTTPRoute specification in the Kubernetes Gateway API. https://github.com/kubernetes-sigs/gateway-api/blob/f24f3a61f398c65ab629da1843cb65fd5ec9419f/apis/v1/httproute_types.go#L148-L209

The new precedence order is:

  1. More specific wins: An Exact match always takes precedence over an All match (where the matches array is empty).
  2. Tie-Breaker (Oldest Rule): If the specificity of the rules is the same (a tie), the rule that was created or deployed first (the older rule) wins.

This approach is both more intuitive and simplifies the implementation of efficient RewriteRule fetching per request. Specifically, when we find an exact match, we no longer need to compare it against less specific, generic rules.

@zetxqx
Copy link
Contributor Author

zetxqx commented Nov 19, 2025

@ahg-g @nirrozenbaum @kfswain

This is ready for review now. I didn't split things up because I feel most of part is relevant but I'm open to split it up if this is too large for review.

The main changes are:

  1. Modify the API: change the conflict precedence rule a bit, more specific match wins then older wins.
  2. Add a separate modelrewrite datastore for dealing with modelrewrite memory store.
  3. Wired up reconciler logic using the modelrewrite datastore.
  4. Wired up director logic using the modelrewrite datastore.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nirrozenbaum nirrozenbaum nirrozenbaum left review comments

@elevran elevran Awaiting requested review from elevran

@robscott robscott Awaiting requested review from robscott

Assignees

No one assigned

Labels

cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@zetxqx @k8s-ci-robot @nirrozenbaum